Bump go-safecast to v2.0.0 closes #23#24
Conversation
jas4711
changed the title
jas4711
force-pushed
the
safecastv2
branch
2 times, most recently
from
0089582 to
b078fe3
Compare
|
The b078fe3 commit was uploaded to Debian experimental, so we get things to build in Debian with go-safecast v2. I'm not sure how to test the patch, I'll get to updating binaries eventually, and maybe find my tkey somewhere for testing... |
|
@ccoVeille could you review if this makes sense from a go-safecast API point of view? |
go.mod
Outdated
|
|
||
| require ( | ||
| github.com/ccoveille/go-safecast v1.1.0 | ||
| github.com/ccoveille/go-safecast v2.0.0 |
There was a problem hiding this comment.
You have an issue here, it should be something like this
| github.com/ccoveille/go-safecast v2.0.0 | |
| github.com/ccoveille/go-safecast/v2 v2.0.0 |
So to make sure everything is fine:
- remove the line here
- run
go mod tidy - run
go get -u github.com/ccoveille/go-safecast/v2 - run
go mod tidy - commit again
There was a problem hiding this comment.
Thank you! Better now?
Debian build system doesn't look at go.mod, so verifying if things works or not was a bit hard for me.
There was a problem hiding this comment.
Let's wait for someone to start the CI to know if it's OK. But otherwise LGTM 👍
There was a problem hiding this comment.
Thanks for review @ccoVeille - just to confirm, does the actual API changes looks correct to you? Getting int conversion wrong here may be have rather bad consequences for this code, so the more eyes can check it the better. I'll see if I somehow can confirm using a real physical Tillitis key that applications linked to this library still works properly.
There was a problem hiding this comment.
I had also reviewed the changes you did, no worries.
I can affirm the ToUintX replaced by Convert [uintX] are all OK.
And than the code behind v1 and v2 is identical and fully tested in my package. I only changed the methods to use.
So the changes are safe in term of code, but of course a full test made by someone with a key would be better.
There was a problem hiding this comment.
Splendid, thank you! Over to upstream...
Closes tillitis#23 Signed-off-by: Simon Josefsson <simon@josefsson.org>
dehanj
left a comment
dehanj
left a comment
There was a problem hiding this comment.
Looks good, tested with real hardware.
Thanks for the help migrating, appreciate it!
3 participants
As used by Debian packaging - should be ready for merge, see review below